Repository navigation
Fix Utf8JsonReader invalid-literal errors: unbounded message, dropped bytes, IndexOutOfRangeException - #134448
caraioniurie47 wants to merge 2 commits into
Conversation
When a true, false or null literal is invalid, the single-segment reader quoted the rest of the buffer in the exception message. The multi-segment reader dropped the last segment's bytes from the message when the data ended inside the literal, threw IndexOutOfRangeException instead of JsonException when nothing had been copied yet, and left BytesConsumed too low when the literal straddled a segment boundary. Quote the literal's bytes up to and including the first mismatch, or all of them when the data ends, in both readers; copy each matching span as soon as it matches; and restore _consumed before throwing. Contributes to dotnet#30706 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
|
@eiriktsarpalis, this is ready for review when there is time. Thanks! |
|
Compare the unchanged multi-segment implementation at this PR's base ( This benchmark reads valid JSON to completion, isolating potential successful-read regressions from exception construction costs. It covers literal-dense arrays ( @EgorBot -amd -arm -commits ec0059c,e566572bee407d689670195e29687c29eb4b2ce6 --filter "LiteralMultiSegmentBenchmarks" --launchCount 3 using BenchmarkDotNet.Running;
using System;
using System.Buffers;
using System.Text;
using System.Text.Json;
using BenchmarkDotNet.Attributes;
BenchmarkSwitcher.FromAssembly(typeof(LiteralMultiSegmentBenchmarks).Assembly).Run(args);
[MemoryDiagnoser]
public class LiteralMultiSegmentBenchmarks
{
private const int ItemCount = 1000;
private byte[] _utf8 = null!;
private ReadOnlySequence<byte> _sequence;
public enum PayloadKind { Literals, Objects }
public enum InputLayout { Span, SingleSegmentSequence, Bytes1, Bytes2, Bytes3, Bytes64, Bytes4096 }
[ParamsAllValues]
public PayloadKind Payload { get; set; }
[ParamsAllValues]
public InputLayout Layout { get; set; }
[GlobalSetup]
public void Setup()
{
var builder = new StringBuilder("[");
for (int i = 0; i < ItemCount; i++)
{
if (i != 0)
{
builder.Append(", ");
}
if (Payload == PayloadKind.Objects)
{
builder.Append(' ', i % 4);
}
// The 17-byte literal-group stride varies alignment at 64-byte and 4 KiB boundaries.
builder.Append(Payload == PayloadKind.Literals
? "true,false,null"
: FormattableString.Invariant($"{{\"id\":{i},\"active\":{(i % 2 == 0 ? "true" : "false")},\"parent\":null,\"name\":\"item {i}\"}}"));
}
builder.Append(']');
_utf8 = Encoding.UTF8.GetBytes(builder.ToString());
int segmentSize = Layout switch
{
InputLayout.Span or InputLayout.SingleSegmentSequence => _utf8.Length,
InputLayout.Bytes1 => 1,
InputLayout.Bytes2 => 2,
InputLayout.Bytes3 => 3,
InputLayout.Bytes64 => 64,
InputLayout.Bytes4096 => 4096,
_ => throw new InvalidOperationException($"Unexpected layout: {Layout}.")
};
if (segmentSize == _utf8.Length)
{
_sequence = new ReadOnlySequence<byte>(_utf8);
}
else
{
var first = new Segment(_utf8.AsMemory(0, segmentSize));
Segment last = first;
for (int offset = segmentSize; offset < _utf8.Length; offset += segmentSize)
{
last = last.Append(_utf8.AsMemory(offset, Math.Min(segmentSize, _utf8.Length - offset)));
}
_sequence = new ReadOnlySequence<byte>(first, 0, last, last.Memory.Length);
}
var reader = Layout == InputLayout.Span ? new Utf8JsonReader(_utf8) : new Utf8JsonReader(_sequence);
int tokens = 0;
int literals = 0;
int splitLiterals = 0;
while (reader.Read())
{
tokens++;
if (reader.TokenType is JsonTokenType.True or JsonTokenType.False or JsonTokenType.Null)
{
literals++;
if (reader.HasValueSequence)
{
splitLiterals++;
}
}
}
int expectedTokens = Payload == PayloadKind.Literals ? 3 * ItemCount + 2 : 10 * ItemCount + 2;
int expectedLiterals = Payload == PayloadKind.Literals ? 3 * ItemCount : 2 * ItemCount;
bool multiSegment = segmentSize < _utf8.Length;
if (reader.BytesConsumed != _utf8.Length || tokens != expectedTokens || literals != expectedLiterals
|| _sequence.IsSingleSegment == multiSegment || (multiSegment && splitLiterals == 0))
{
throw new InvalidOperationException(
$"Invalid coverage for {Payload}/{Layout}: {reader.BytesConsumed} bytes, {tokens} tokens, {literals} literals, {splitLiterals} split literals.");
}
}
[Benchmark]
public long Read()
{
var reader = Layout == InputLayout.Span ? new Utf8JsonReader(_utf8) : new Utf8JsonReader(_sequence);
while (reader.Read())
{
}
return reader.BytesConsumed;
}
private sealed class Segment : ReadOnlySequenceSegment<byte>
{
public Segment(ReadOnlyMemory<byte> memory) => Memory = memory;
public Segment Append(ReadOnlyMemory<byte> memory)
{
var next = new Segment(memory) { RunningIndex = RunningIndex + Memory.Length };
Next = next;
return next;
}
}
}Note This benchmark and request were generated by GitHub Copilot at the reviewer's direction. |
|
Run the existing dotnet/performance multi-segment reader benchmarks against this PR's exact base and head, on the same architectures as the focused literal benchmark above:
These are genuine Coverage caveat: in the current @EgorBot -amd -arm -commits ec0059c,e566572bee407d689670195e29687c29eb4b2ce6 --filter "*System.Text.Json.Tests.Perf_Segment.ReadMultiSegmentSequence" "*System.Text.Json.Tests.Perf_Reader.ReadMultiSpanSequenceEmptyLoop" --launchCount 3 Note This investigation and benchmark request were generated by GitHub Copilot at the reviewer's direction. |
|
@eiriktsarpalis, both EgorBot requests failed before any benchmark ran (EgorBot/Benchmarks#657, EgorBot/Benchmarks#658): the agent checks out each Without Thanks. |
When
Utf8JsonReaderfinds an invalidtrue,falseornull, the exception depends on how the input is segmented, and in some layouts it is not aJsonExceptionat all. Measured on .NET 10.0.12:byte[], or a one-segmentReadOnlySequence): the message quotes everything to the end of the buffer.CheckLiteralpassesThrowInvalidLiteralthe spanConsumeLiteralgave it,_buffer.Slice(_consumed), sotb:gives'tb:' is an invalid JSON literal, and a 1,000,007-byte payload starting[falsXgave a 1,000,105-character message.IndexOutOfRangeException, e.g. segments"t",""or"[","tr","".CheckLiteralMultiSegmentcopied a span into its message buffer only afterGetNextSpan()returned the next one, so nothing was copied, andGetInvalidLiteralMultiSegmentreadspan[0]of an empty span.falsread one byte per segment gives'fal'.BytesConsumedafter the exception can be too low when the literal straddles a segment boundary:[truas two segments split after[tor[trreports 0, while thebyte[]and the other splits that throw aJsonExceptionreport 1. The throw path restored_totalConsumedbut not_consumed, whichGetNextSpan()had reset.Change. Both paths now quote the literal's bytes up to and including the first mismatching byte, or every byte present when the data ends, so the quote is at most the literal's length and the same for every segmentation.
CheckLiteralMultiSegmentcopies each span's matching bytes as soon as they match, which removes theAmountToWritehelper, and restores_consumedbefore throwing. HowBytePositionInLineandLineNumberare computed is unchanged.This addresses the literal half of #30706. The comment-handling differences listed there (
{/,/+and others, withJsonCommentHandling.SkipandAllow) are not changed here and remain. Nothing is thrown that was not thrown before, apart fromJsonExceptionreplacingIndexOutOfRangeException.Tests
InvalidLiteralVariousSegmentSizesadded toUtf8JsonReaderTests.MultiSegment.cs, in the shape ofInvalidJsonNumberVariousSegmentSizes: 11 inputs, each read as abyte[], a one-segmentReadOnlySequence, one byte per segment (the helper adds an empty last segment), and every one- and two-splitReadOnlySequence, including splits at the end so an empty last segment is covered. Each read assertsLineNumber,BytePositionInLine,BytesConsumed,Positionand the quoted literal.IndexOutOfRangeExceptionfort,'nu','fal'and'tr'for the dropped bytes,'tb:'and'nulL]'for the whole-buffer quote. With it all 11 pass.System.Text.Json.Tests,System.Text.Json.SourceGeneration.Roslyn4.4.TestsandSystem.Text.Json.SourceGeneration.Roslyn3.11.Testspass in both target frameworks.Performance
BenchmarkDotNet 0.15.2, Windows 11 x64: a Release build of
mainand the same build with this change toSystem.Text.Json, each in its own Release testhost, compared with--coreRun. The benchmark reads an array of 3000 elements to the end:Literalsistrue,false,nullrepeated,Objectsis{"id":n,"active":true|false,"parent":null,"name":"item n"}. Segment size 0 is abyte[]; with 3 every literal straddles segments, the path whose copying changed. Three runs, one launch per build and then three, mean ± SD in µs:main(runs 1 / 2 / 3)By mean, the lower build changes between runs in every row except
Objects/3, where this branch is lower in all three runs by 0.6% to 3%.Objects/64, higher on this branch in runs 1 and 2, was rerun alone with twenty launches per build, once in each host order: this branch 485.7 ± 18.3 againstmain481.0 ± 19.4 µs, and 490.9 ± 16.5 against 491.0 ± 15.3 µs. Its spread is about 3.5% of the mean and neither build is consistently ahead. Thebyte[]rows, whose only change is in the throw path, move by up to 4% between builds within a run.Contributes to #30706
Note
AI-generated, written at my direction and reviewed by me before posting. Everything ran on Windows 11 x64 (build 26200). The measurements in the first list ran on the installed .NET 10.0.12 release with a scratch console app; the two crash inputs and the 1,000,105-character message were re-run under the Release
corerunofmainbuilt locally, with the same result, and under this branch's, where they giveJsonExceptionand a 104-character message.Positionafter the exception was measured the same way, on all three builds, for every two-segment split of[tru,[nulL],{"":trand[ trux]. The tests ran on Debug libraries with a Checked runtime; the benchmark as described under Performance.